Repository navigation
fix(integrations): pre-approve Cursor Band tools and surface agent crashes - #750
Conversation
AlexanderZ-Band
changed the title
AlexanderZ-Band
left a comment
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Four verified in-scope findings: startup-cancellation diagnostics, custom-tool coverage, failure-triggered cleanup before stdout EOF, and custom-name ownership collision. Fixing under the requested --fix workflow. GitHub rejected REQUEST_CHANGES because the authenticated user is this PR author; submitting as COMMENT.
…approve-bands-mcp-tools-and-surfa-INT-1644
…-and-surfa-INT-1644
amit-gazal-band
left a comment
amit-gazal-band
left a comment
There was a problem hiding this comment.
Requesting changes for one blocker: a background helper that inherits stderr hides the agent's crash and makes every stop() take 5s (inline on stderr.py:53, reproduced with a real subprocess).
Everything else inline is an FYI: worth a look, but it doesn't block merge.
The last two commits already fixed two earlier findings: a cancelled start() is no longer logged as a crash, and the e2e test now uses the shared is_cursor_band_tool helper instead of its own copy.
Checks at the reviewed commit: ruff check/format pass. The unit suite (--extra dev, excluding integration/e2e/mcp) had 2 failures, both in test_strands_adapter.py; they also fail on main, so this PR didn't cause them. The live e2e lanes were not run.
amit-gazal-band
left a comment
amit-gazal-band
left a comment
There was a problem hiding this comment.
Approving. I left three non-blocking comments: two are worth a follow-up, and one is a very low-risk edge case.
…-and-surfa-INT-1644 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjcaAV748RqtWpvsutxupX
2 participants
Summary
Cursor's registered Band MCP tools receive per-call approval during a live turn. Native and other-server tools retain the configured approval policy. Unexpected ACP subprocess exits report their exit code and a bounded stderr tail, including when a background helper keeps stderr open.
Fixes INT-1644.
Changes
Latest review fixes on 8de90d0
The three additional nonblocking comments were confirmed and fixed:
Validation: Ruff check, Ruff format check and Pyrefly passed. Full prescribed unit gate: 7,926 passed, 170 skipped, 81 warnings, 270.94s; integration and live E2E directories excluded. Focused ACP/adapter gate: 600 passed, 10 skipped (existing opt-in real Codex ACP E2E cases). Six new regression rows failed before the fixes and pass afterward; foreign-MCP provenance has an additional regression. Tests exercise real in-process ACP and stdio subprocess boundaries. CI on this new head is pending; historical CI and live acceptance evidence below is unchanged.
Earlier review disposition
All nine new review threads were evaluated, replied to, and resolved. Seven findings were confirmed and fixed; the canonicalization finding was partly right and its small pre-existing external-MCP turn-effect defect was fixed. The proposed lock release was rejected because it permits overlapping teardown/respawn of shared runtime state. The duplicate injected-MCP text-reply claim does not hold on the current reply contract.
Validation on 4de725e
Live acceptance remains incomplete
The current repair workflow run used repo-root
.env.testand disabled retries: 1 failed, 1 deselected in 72.69s. Agent provisioning returned platform HTTP 500 Internal Server Error, before Cursor or the workflow body ran. No pytest retry was performed; the generated REST client retains its default two HTTP retries. This is not a live pass.Earlier focused real-platform Cursor repair/restart validation on
61c575ed: 2 passed, retries disabled. That evidence predates these review fixes.The earlier complete backends lane was unsuccessful on its first attempt: 146 passed, 396 skipped, 1 failed, 1 fixture error, 1 recovered retry. A second-user authorized-member case remains blocked by the recorded
403 plan_requiredentitlement failure. These failures are not erased by local/CI passes or focused rechecks.Shared Phoenix channel recovery is tracked separately in INT-1700; attribution of the historical Cursor timeout to that defect remains unproven.
Provisioning failure investigation
Independent registration with HTTP retries explicitly disabled reproduced the original 500. Both payloads pass the live OpenAPI schema; isolated toolkit and workflow experiments exclude serialization and model execution as the source. Subsequent calls returned a transport timeout, 403
plan_required, and 201 success, so the dev platform boundary is intermittent rather than the request being permanently invalid. The current sibling platform controller has an empirically confirmed error-handling gap: an upstream{:error, {:badrpc, reason}}raisesCaseClauseErrorinstead of reaching the existing HTTP 503 fallback. Its attribution to the observed 500 remains unproven without backend logs.